Fix broken network cookbook with newest konnektor updates - #324
hannahbaumann wants to merge 13 commits into
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
|
hannahbaumann
changed the title
hannahbaumann
changed the title
| @@ -0,0 +1,853 @@ | |||
| { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Rephrased this!
| @@ -0,0 +1,853 @@ | |||
| { | |||
There was a problem hiding this comment.
A little bit more prose here would be good, i.e. "we start by loading our LigandNetwork, which can be found in mst_network_tyk2.json..."
Reply via ReviewNB
| @@ -0,0 +1,853 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #7. plot_atommapping_network(planned_network)
If you capture the figure, I think it'll only plot the output once
Reply via ReviewNB
There was a problem hiding this comment.
Good point, I changed this!
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
I mostly reviewed for content - overrall looks good to me, just a couple of small comments on the amount of details that are in the text.
| - git+https://github.com/OpenFreeEnergy/cinnabar@0.6.0 # Use conda-forge v0.6.0 after new openfe release updates pins on cinnabar | ||
| - git+https://github.com/OpenFreeEnergy/kartograf@v2.0.0 # Use conda-forge v2.0 after new openfe release updates pins on kartogra | ||
| - git+https://github.com/OpenFreeEnergy/konnektor@v0.4.0 # use conda-forge v0.4.0 after openfe conda-forge release | ||
| - git+https://github.com/OpenFreeEnergy/konnektor@avoid_edges_concatenator # use conda-forge v0.4.0 after openfe conda-forge release |
There was a problem hiding this comment.
Thanks, I changed this!
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
Make it clear this is specific to the hybrid topology protocol currently (the way we get the protocol and build the transformations) but the process of identifying missing results and creating new transformations should apply to other RBFE protocols?
Reply via ReviewNB
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
"we can determine which planned transformations" -> "we can determine which transformations" planned again feels awkward.
"Every planned ligand is kept," -> "We retain all input ligands from the planned network, including those for which no transformation ..."
Reply via ReviewNB
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
"A transformation counts as successful only when both its complex and solvent legs are completed successfully." We should make it clear that this is specific to the Hybrid Topology Protocol and that the settings helper function below is specific for that one as well or consider making it general?
Reply via ReviewNB
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #32. is_complex = any(
We could use the contains api https://github.com/OpenFreeEnergy/gufe/blob/b338c869ec31e2570249f3aef38ab6121ac99ea1/src/gufe/chemicalsystem.py#L128
so it would be:
is_complex = state_a.contains(ProteinComponent)
Reply via ReviewNB
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #4. mapper: AtomMapper,
To consider, should this have the same API as the network generators if this would eventually go into Konnektor and support multiple mappers and a scorer?
Reply via ReviewNB
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #5. ("lig_ejm_31", "lig_ejm_46"),
Is it intentional to generate the same edges again that we faked failing? We also don't do anything with new edges by name should we check this matches what we intened?
Reply via ReviewNB
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #2. charge_a = Chem.rdmolops.GetFormalCharge(mapping.componentA.to_rdkit())
We can use mapping.get_alchemical_charge_difference()
Reply via ReviewNB
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #14. settings = copy.deepcopy(reference_protocol.settings)
we could use reference_protocol.settings.unfrozen_copy()
Reply via ReviewNB
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #24. protein = next(
For the protein and solvent consider using get_components_of_type
Reply via ReviewNB
| @@ -0,0 +1,812 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #39. cofactors = {
For cofactors, we could also use get_components_of_type SmallMoleculeComponent and then remove those in the mapping, this feels a little safer than going on the cofactor label?
Reply via ReviewNB
4 participants
No description provided.